Skip to content

fix(node): carry full owner DID on ref-update wire event (#144)#145

Open
Gravirei wants to merge 12 commits into
Gitlawb:mainfrom
Gravirei:fix/issue-144-full-did-wire-slug
Open

fix(node): carry full owner DID on ref-update wire event (#144)#145
Gravirei wants to merge 12 commits into
Gitlawb:mainfrom
Gravirei:fix/issue-144-full-did-wire-slug

Conversation

@Gravirei

@Gravirei Gravirei commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Carry the full owner DID on the ref-update wire event so the feed gate can distinguish different DID methods that share the same trailing segment (e.g. did:web:host:alice vs did:gitlawb:alice).

Motivation & context

Refs #144

The ref-update wire slug was previously constructed from the last colon-segmented component of the owner DID ({owner_did.split(":").last}/{name}), which is lossy. Two owners with different DID methods but the same trailing segment would produce identical slugs, causing the feed gate to over-drop remote rows for anonymous callers.

This PR is the plumbing half of #144: it carries the full owner DID on the wire and stores it in the database so a follow-up can teach the feed gate to use did_matches for proper disambiguation. The gate itself (the second half of #144) will land in a separate change on top of this.

Kind of change

  • Bug fix
  • Feature
  • Security fix
  • Docs
  • Tests / CI
  • Refactor (no behavior change)
  • Breaking or protocol change (issue required first)

What changed

crates/gitlawb-node (gitlawb-node):

  • p2p/mod.rs: Added owner_did: Option<String> to RefUpdateEvent with #[serde(default)] for backward compat with older peers that do not include the field
  • db/mod.rs: Added owner_did: Option<String> to ReceivedRefUpdate, added migration v10 to add owner_did TEXT column and idx_ref_updates_owner index to the received_ref_updates table, updated all SELECT/INSERT queries to include the column
  • api/repos.rs: Set owner_did: Some(record.owner_did) when publishing ref-update events over gossip; added owner_did to the HTTP sync notify body
  • api/peers.rs: Added owner_did to NotifyRequest and wired it through to ReceivedRefUpdate

How a reviewer can verify

DATABASE_URL=postgresql://gitlawb:changeme@172.19.0.2:5432/gitlawb cargo test -p gitlawb-node
cargo fmt --check
cargo clippy --workspace --all-targets -- -D warnings

Before you request review

  • Scope is one logical change; no unrelated churn
  • cargo test --workspace passes locally (327/327)
  • New behavior is covered by tests (required for fixes)
  • cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
  • Commit titles use Conventional Commits (fix(...), test(...), style(...))
  • Docs / .env.example updated if behavior or config changed (or N/A)
  • Checked existing PRs so this is not a duplicate

Protocol & signing impact

This is a backward-compatible wire-format addition: the new owner_did field is #[serde(default)], so events from older peers deserialize without it (owner_did: None). The repo field format is unchanged. Migration v10 adds the column to existing databases; ADD COLUMN IF NOT EXISTS is idempotent for re-runs.

  • Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
  • Discussed in an issue before implementation
  • Backward-compatible with existing nodes and previously signed history

Summary by CodeRabbit

  • New Features

    • Ref-update notifications and event listings can now include an optional owner_did when available.
    • Peer-to-peer sync notifications now propagate the owner DID into published ref-update events.
    • GraphQL ref-updates and subscriptions expose owner_did as optional (omittable).
  • Bug Fixes

    • Owner DID is now stored and returned consistently across sync, peer updates, API event endpoints, and persisted ref-update history.
    • Backward compatibility is preserved when owner_did is missing or null in incoming notifications.

@github-actions github-actions Bot added the needs-tests Source changed without accompanying tests (advisory) label Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution. A couple of things will help us review this faster:

  • This changes Rust source but no tests changed. Tests are required for fixes and strongly encouraged for features.

See CONTRIBUTING.md. Update the PR and these notes will clear automatically.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an optional owner_did field across ref-update request, event, persistence, GraphQL, and events API paths, and threads it through peer notifications, gossip handling, database queries, and endpoint tests.

Changes

Owner DID propagation

Layer / File(s) Summary
HTTP sync/notify and peer fan-out
crates/gitlawb-node/src/api/peers.rs, crates/gitlawb-node/src/api/repos.rs
Adds owner_did to sync notify requests, forwards it through peer notification payloads, and includes it in ref-update event publication and tests.
Gossip RefUpdateEvent owner_did propagation
crates/gitlawb-node/src/p2p/mod.rs, crates/gitlawb-node/src/state.rs
Adds owner_did to gossip ref-update events, persists it on receipt, and updates the broadcast payload contract.
ReceivedRefUpdate schema and DB storage
crates/gitlawb-node/src/db/mod.rs
Adds owner_did to the stored ref-update shape, migration, queries, and DB tests.
Events and GraphQL responses
crates/gitlawb-node/src/api/events.rs, crates/gitlawb-node/src/graphql/*.rs, crates/gitlawb-node/src/test_support.rs
Returns owner_did in ref-update events and GraphQL ref-update responses, updates schema mapping, and adds endpoint tests.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

Possibly related PRs

  • Gitlawb/node#39: Modifies the same /api/v1/sync/notify path and NotifyRequest/ReceivedRefUpdate flow.
  • Gitlawb/node#68: Touches the same git_receive_pack path in api/repos.rs and uses record.owner_did in adjacent logic.
  • Gitlawb/node#72: Changes the same peer notification payload-building path in api/repos.rs.

Suggested labels: sev:medium, subsystem:api

Suggested reviewers: jatmn, kevincodex1

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: carrying the full owner DID on ref-update wire events.
Description check ✅ Passed The description follows the template well, covering summary, motivation, changes, verification, and protocol impact.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@beardthelion beardthelion added crate:node gitlawb-node — the serving node and REST API kind:bug Defect fix — wrong or unsafe behavior subsystem:peers Peer announce, discovery, and registry labels Jul 2, 2026
@github-actions github-actions Bot removed the needs-tests Source changed without accompanying tests (advisory) label Jul 2, 2026
@Gravirei Gravirei marked this pull request as ready for review July 2, 2026 03:16

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
crates/gitlawb-node/src/test_support.rs (1)

1969-2043: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a shared builder for ReceivedRefUpdate test fixtures.

Both tests construct near-identical ReceivedRefUpdate literals inline. A shared helper (similar to the update(...) fixture already used in db/mod.rs tests) would reduce duplication and make future field additions (like owner_did) less error-prone to keep in sync across test files.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/test_support.rs` around lines 1969 - 2043, The
`events_returns_inserted_ref_updates` and `events_limit_respects_limit_param`
tests duplicate the same `crate::db::ReceivedRefUpdate` setup inline, so add a
shared test fixture/builder for `ReceivedRefUpdate` in `test_support.rs` and use
it in both cases. Model it after the existing `update(...)` helper used in the
db tests, and make sure the helper accepts overrides for fields like `repo`,
`owner_did`, `new_sha`, and timestamps so future schema changes stay consistent
across tests.
crates/gitlawb-node/src/db/mod.rs (1)

3919-4077: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert owner_did in the filtered-by-repo test too.

list_ref_updates_filtered_by_repo (lines 4027-4058) inserts records with distinct owner_did values but only asserts id/count, not owner_did, unlike the sibling list_repo_ref_updates_filters_by_repo test which does check it. Adding that assertion would close a small gap in coverage for the exact field this PR introduces.

♻️ Proposed test strengthening
         let filtered = db
             .list_ref_updates_filtered(Some("ownerA/proj"), 100)
             .await
             .unwrap();
         assert_eq!(filtered.len(), 1);
         assert_eq!(filtered[0].id, "u5");
+        assert_eq!(filtered[0].owner_did.as_deref(), Some("did:key:zA"));
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/db/mod.rs` around lines 3919 - 4077, The
list_ref_updates_filtered_by_repo test is missing coverage for the new owner_did
behavior. Update ref_update_db_tests::list_ref_updates_filtered_by_repo to
assert the returned row’s owner_did matches the inserted value, similar to
list_repo_ref_updates_filters_by_repo, so the filtered path verifies both repo
filtering and owner_did preservation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 1969-2007: The ref-update events payload returned by the events
API is still missing owner_did, so the current test only verifies persistence in
Postgres. Update the ref-update feed serialization in the events response path
to include owner_did, using the relevant event model/handler that backs
events_router and GET /api/v1/events/ref-updates, and then extend
events_returns_inserted_ref_updates to assert events[0]["owner_did"] matches the
inserted value.

---

Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 3919-4077: The list_ref_updates_filtered_by_repo test is missing
coverage for the new owner_did behavior. Update
ref_update_db_tests::list_ref_updates_filtered_by_repo to assert the returned
row’s owner_did matches the inserted value, similar to
list_repo_ref_updates_filters_by_repo, so the filtered path verifies both repo
filtering and owner_did preservation.

In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 1969-2043: The `events_returns_inserted_ref_updates` and
`events_limit_respects_limit_param` tests duplicate the same
`crate::db::ReceivedRefUpdate` setup inline, so add a shared test
fixture/builder for `ReceivedRefUpdate` in `test_support.rs` and use it in both
cases. Model it after the existing `update(...)` helper used in the db tests,
and make sure the helper accepts overrides for fields like `repo`, `owner_did`,
`new_sha`, and timestamps so future schema changes stay consistent across tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6d0fa77a-014d-4a04-af46-9ef99e20fe27

📥 Commits

Reviewing files that changed from the base of the PR and between 466a550 and b988780.

📒 Files selected for processing (5)
  • crates/gitlawb-node/src/api/peers.rs
  • crates/gitlawb-node/src/api/repos.rs
  • crates/gitlawb-node/src/db/mod.rs
  • crates/gitlawb-node/src/p2p/mod.rs
  • crates/gitlawb-node/src/test_support.rs

Comment thread crates/gitlawb-node/src/test_support.rs

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The write-side plumbing is right: the full DID comes from the local repo record, it's threaded through both the gossip and HTTP-notify paths, and the #[serde(default)] / nullable column keeps older peers compatible. CodeRabbit's feed-serialization ask is genuinely addressed too. Two things block merge, and the first is a hard one: the schema change breaks every existing node on upgrade, and the PR closes #144 without fixing #144's symptom.

Findings

  • [P1] Ship owner_did as a new migration version, not appended to v1
    crates/gitlawb-node/src/db/mod.rs:542
    The ALTER TABLE ... ADD COLUMN owner_did and idx_ref_updates_owner were added inside Migration { version: 1 }, but run_migrations skips any version already recorded in schema_migrations, and the catalogue comment right above the array says new schema must be a new version (v2, v3, ...). Every existing node has v1 recorded, so on upgrade the column is never created; insert_ref_update (binds $12) and all three list_* SELECTs then reference a missing column and error, which stops ref-update ingestion on both the gossip and notify paths and 500s the events feed. Fresh databases pass because they run v1 from scratch, which is why CI is green. I reproduced it against the real migration runner: an existing node (v1..v9 recorded, no owner_did) upgraded to this build leaves the column absent and the insert/select fail; moving both statements into a new Migration { version: 10 } makes the same upgrade round-trip cleanly.

  • [P2] Don't close #144 on this PR; nothing consumes owner_did yet
    crates/gitlawb-node/src/api/events.rs:38
    #144 has two halves: carry the full owner DID (done here) and make the feed gate use it to stop the over-drop. Nothing in this diff reads owner_did for a visibility decision; it's stored and echoed in the feed JSON only, and idx_ref_updates_owner is an index no query uses. The gate that over-drops isn't in this branch, so merging this auto-closes #144 while the reported symptom persists. Keep the plumbing, but change "Closes #144" to "Refs #144" and land the gate consumption (the did:key-aware match #144 describes) in the follow-up that sits on top of the gate.

  • [P3] Populate owner_did on the other read surfaces, or hold the JSON change
    crates/gitlawb-node/src/graphql/query.rs:61, crates/gitlawb-node/src/api/events.rs:117
    The field lands on the global REST feed but the GraphQL ref_updates resolver drops it (RefUpdateType has no such field) and the repo-scoped list_repo_events gossip block omits it. No consumer reads it yet so there's no impact today, but the asymmetry is the same drift the duplicated four-copy SELECT column list invites, and it becomes a trap once something depends on the field. Either populate all three surfaces or defer the JSON exposure until the gate needs it. While here, add a migration upgrade-path test (seed schema_migrations at v1, run migrations, assert the column round-trips) so the v1-append class can't recur.

The direction is right and the receive/publish wiring is sound; once the migration moves to its own version this is close.

@Gravirei Gravirei requested a review from beardthelion July 2, 2026 05:27
@beardthelion beardthelion dismissed their stale review July 2, 2026 13:36

P1 migration break resolved on 72ccb28; re-reviewed the new head, posting a fresh review.

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed on 72ccb28d. The P1 migration break is resolved and I verified it by execution: the owner_did DDL now lives in a new Migration{version:10}, and on a simulated existing node (schema_migrations at v1..v9, received_ref_updates without the column) migrate() adds the column and index and records v10. Putting the DDL back inside the already-applied v1 makes that same check fail (the column is never re-added), so the guard is load-bearing. Wire compat (#[serde(default)] Option<String>) and the DB plumbing are sound, and "Refs #144" is correct. Four items to fold in.

Findings

  • [P2] Drop the premature idx_ref_updates_owner index, or defer it to the gate PR
    crates/gitlawb-node/src/db/mod.rs:833
    Nothing reads received_ref_updates by owner_did yet: every reference is a projection, no WHERE/JOIN/ORDER. Migrations run inside a transaction so CREATE INDEX CONCURRENTLY isn't available here (mod.rs:426), which means this index builds under a write-blocking lock at startup on exactly the populated nodes this PR targets, for no current benefit. Ship the column now and add the index in #143 next to the query that reads it.

  • [P2] Expose owner_did on the repo-scoped events feed
    crates/gitlawb-node/src/api/events.rs:116
    list_ref_updates returns owner_did now, but the gossip_events block in list_repo_events omits it, so GET /api/v1/repos/{owner}/{repo}/events silently drops the field. Add "owner_did": u.owner_did to that json! block to match line 38.

  • [P2] Carry owner_did through the GraphQL query and subscription
    crates/gitlawb-node/src/graphql/types.rs:52
    RefUpdateType and RefUpdateBroadcast (state.rs:12) have no owner_did, so the ref_updates query (query.rs:61) and the subscription drop it while REST exposes it. Add owner_did: Option<String> to both structs and populate it at query.rs, subscription.rs, and the broadcast send in repos.rs.

  • [P2] Add an existing-node upgrade test for v10
    crates/gitlawb-node/src/db/mod.rs:3200
    migration_v10_creates_owner_did_column runs only a fresh v1..v10 chain, which can't catch DDL appended to an already-applied version. Seed schema_migrations with v1..v9 and a received_ref_updates without the column, then migrate() and assert the column, index, and v10 row appear. I ran exactly this against the head and it passes; the same body fails when the DDL sits back in v1, so it is the missing regression guard, not a live bug.

One forward note for #143, not a blocker here: the wire owner_did is attacker-controlled (the peer signature authenticates the relaying node, not this field) and is stored verbatim. Every authorization path today reads the trusted record.owner_did from the local repo row, so nothing consumes the wire value yet, but the gate must cross-check it against a trusted source and must not fall back to the lossy last-segment / did_matches matcher the full DID exists to avoid.

@Gravirei migration fix verified, thanks. The above are plumbing-consistency and deploy-window items.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/gitlawb-node/src/db/mod.rs (2)

3207-3218: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Assert the column is actually nullable.

The tests fetch is_nullable but only assert the name/type. Since older peers can omit owner_did, please assert "YES" so a future NOT NULL change is caught.

Proposed fix
         assert_eq!(col.0, "owner_did");
         assert_eq!(col.1, "text");
+        assert_eq!(col.2, "YES");

Apply the same assertion in both migration tests.

Also applies to: 3274-3284

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/db/mod.rs` around lines 3207 - 3218, The migration
tests for received_ref_updates currently verify owner_did exists and is text,
but they ignore the fetched is_nullable value. Update both migration test blocks
that query information_schema.columns in db::mod to assert the third tuple field
from sqlx::query_as is "YES" alongside the existing name/type checks, so changes
to nullable behavior are caught.

2103-2107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Preserve later owner_did enrichment on duplicate ref-update IDs.

With ON CONFLICT(id) DO NOTHING, a mixed-version network can store the older None payload first and ignore a later duplicate carrying the full owner DID, leaving the feed unable to disambiguate that event.

Proposed fix
              (id, node_did, pusher_did, repo, ref_name, old_sha, new_sha, timestamp,
               cert_id, received_at, from_peer, owner_did)
              VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12)
-             ON CONFLICT(id) DO NOTHING",
+             ON CONFLICT(id) DO UPDATE
+             SET owner_did = COALESCE(received_ref_updates.owner_did, EXCLUDED.owner_did)
+             WHERE received_ref_updates.owner_did IS NULL
+               AND EXCLUDED.owner_did IS NOT NULL",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/db/mod.rs` around lines 2103 - 2107, The INSERT into
received_ref_updates currently uses ON CONFLICT(id) DO NOTHING, which prevents a
later duplicate from enriching an earlier row with owner_did. Update the upsert
logic in the received ref-update insert path so duplicates with the same id can
fill in a missing owner_did while still avoiding overwriting existing non-null
data. Refer to the received_ref_updates insert statement and the surrounding db
write function that persists ref updates.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/db/mod.rs (1)

4096-4129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a same-slug owner collision regression.

This test uses different repo values, so it does not cover the PR’s core case: two updates with the same trailing repo slug but different full owner_did values. Add a row pair like alice/myrepo + did:web:host:alice and alice/myrepo + did:gitlawb:alice.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/gitlawb-node/src/db/mod.rs` around lines 4096 - 4129, The current
`list_repo_ref_updates_filters_by_repo` test only verifies filtering by full
repo name, not the same-slug owner collision case. Update this test in `db` to
insert two ref updates with the same trailing repo slug but different full owner
identifiers, using the existing `insert_ref_update`, `update`, and
`list_repo_ref_updates` helpers, and assert the query returns the correct row
for each full repo owner path. Keep the original repo-filter assertions if
helpful, but add coverage for the `owner_did` collision scenario the PR is
targeting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/gitlawb-node/src/api/events.rs`:
- Line 130: The event payload in list_repo_events is only populating owner_did
for gossipsub events, so local cert events in the same feed remain inconsistent.
Update the local event mapping in events.rs to also set owner_did from
record.owner_did, keeping the repo-scoped payload shape consistent with the
existing gossipsub path and preserving consumers that dedup or gate on the full
owner DID.

---

Outside diff comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 3207-3218: The migration tests for received_ref_updates currently
verify owner_did exists and is text, but they ignore the fetched is_nullable
value. Update both migration test blocks that query information_schema.columns
in db::mod to assert the third tuple field from sqlx::query_as is "YES"
alongside the existing name/type checks, so changes to nullable behavior are
caught.
- Around line 2103-2107: The INSERT into received_ref_updates currently uses ON
CONFLICT(id) DO NOTHING, which prevents a later duplicate from enriching an
earlier row with owner_did. Update the upsert logic in the received ref-update
insert path so duplicates with the same id can fill in a missing owner_did while
still avoiding overwriting existing non-null data. Refer to the
received_ref_updates insert statement and the surrounding db write function that
persists ref updates.

---

Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 4096-4129: The current `list_repo_ref_updates_filters_by_repo`
test only verifies filtering by full repo name, not the same-slug owner
collision case. Update this test in `db` to insert two ref updates with the same
trailing repo slug but different full owner identifiers, using the existing
`insert_ref_update`, `update`, and `list_repo_ref_updates` helpers, and assert
the query returns the correct row for each full repo owner path. Keep the
original repo-filter assertions if helpful, but add coverage for the `owner_did`
collision scenario the PR is targeting.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9f70fb07-7129-4c96-9e77-7b2943106a56

📥 Commits

Reviewing files that changed from the base of the PR and between 72ccb28 and 59c26a7.

📒 Files selected for processing (7)
  • crates/gitlawb-node/src/api/events.rs
  • crates/gitlawb-node/src/api/repos.rs
  • crates/gitlawb-node/src/db/mod.rs
  • crates/gitlawb-node/src/graphql/query.rs
  • crates/gitlawb-node/src/graphql/subscription.rs
  • crates/gitlawb-node/src/graphql/types.rs
  • crates/gitlawb-node/src/state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/gitlawb-node/src/api/repos.rs

Comment thread crates/gitlawb-node/src/api/events.rs Outdated
@Gravirei Gravirei requested a review from beardthelion July 2, 2026 16:20
beardthelion
beardthelion previously approved these changes Jul 2, 2026

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed on 63ab6015. All four items from my last pass are in, CodeRabbit's local-cert-events fix landed, and I verified the security- and deploy-relevant paths by execution:

  • Migration, both ways: on a simulated existing node (v1..v9 recorded, no owner_did column), migrate() adds the column and records v10; moving that DDL back into the already-applied v1 makes migration_v10_existing_node_upgrade fail (RowNotFound) while the fresh-DB test still passes, so the upgrade guard is load-bearing. A pre-upgrade row (owner_did NULL, since the ALTER has no default) round-trips through the feed as JSON null, no 500. The index is correctly deferred to #143.

  • Feed provenance, executed: the repo-scoped feed's local block carries the trusted record.owner_did, and the gossip block echoes the stored wire value verbatim, which I confirmed with a spoofed owner_did surfacing unchanged (display-only, nothing filters on it). All four received_ref_updates SELECTs project owner_did, so the shared row mapper never hits a missing column. The global feed and GraphQL (query, subscription, broadcast) carry the field through as a passthrough.

  • [P3] The stored received_ref_updates.owner_did is attacker-controlled (the peer signature authenticates the relaying node, not this field, and notify_sync accepts it unsigned by default) and is echoed verbatim, as the spoof test shows. It is display-only and nothing gates on it, so not a break. A short comment at the two ingest sites (p2p/mod.rs, api/peers.rs:398) marking the column untrusted would keep #143's DID-aware feed gate from consuming it as trusted; that gate must reconcile it against the repo slug or a verified cert.

@Gravirei the P2s and the migration all check out, thanks. @kevincodex1 this is ready.

@kevincodex1 kevincodex1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevincodex1

Copy link
Copy Markdown
Contributor

hello @Gravirei kindly rebase to main and fix conflicts

Copilot AI review requested due to automatic review settings July 5, 2026 16:40
@Gravirei Gravirei force-pushed the fix/issue-144-full-did-wire-slug branch from 63ab601 to b46005b Compare July 5, 2026 16:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Gravirei Gravirei requested a review from kevincodex1 July 5, 2026 16:43
@beardthelion beardthelion dismissed their stale review July 6, 2026 03:23

Stale after the rebase to b46005b; re-reviewing the new head.

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed on b46005bf. The owner_did work (#144) is solid: owner_did is carried on all four emit paths, the shared received_ref_updates mapper and migration v10 line up, and the feed gate never trusts the stored value. But the rebase onto 0.5.0 pulled api/ipfs.rs backward relative to main and reintroduces an unauthenticated availability regression, so I can't approve this head.

Findings

  • [P1] Restore the pre-walk object-existence check on GET /ipfs/{cid}
    crates/gitlawb-node/src/api/ipfs.rs (get_by_cid)
    Current main (via #133, 466a550) runs store::object_type and continues before the reachability walk, so a random CID can't trigger a full-history walk on repos that don't hold the object. This branch removes that guard and runs allowed_blob_set_for_caller (git rev-list --all + a per-commit ls-tree) before read_object. The route is optional_signature and unthrottled, and get_by_cid iterates the full list_all_repos(), so one anonymous request with an absent CID walks every public path-scoped repo's history. I reproduced it: an anon request for an object not in the repo fires the walk on this head and fires zero on main. Restore main's object_type pre-filter, or gate the walk behind an indexed proof the object is present.

  • [P2] Drop the /ipfs/{cid} change from this PR
    crates/gitlawb-node/src/api/ipfs.rs
    d995c4de is a ~150-line ipfs gate revision unrelated to #144, and main already carries #133. Rebasing onto current main to drop it (keeping main's ipfs.rs) removes both the P1 and the scope creep in one move.

  • [P2] Correct the docstring claiming owner_did feeds the feed gate
    crates/gitlawb-node/src/p2p/mod.rs:42
    The comment says owner_did was added "so the feed gate can distinguish different DID methods that share the same trailing segment," but visibility::ref_update_row_names_repo matches on row_repo plus the local record and collapses both via .split(':').next_back(), never reading the wire owner_did. The field is additive display/storage today. Either wire owner_did into the matcher or fix the doc so it doesn't imply that collision is closed.

  • [P3] Exercise the real v9 to v10 upgrade over populated data
    crates/gitlawb-node/src/db/mod.rs (migration_v10_creates_owner_did_column)
    The test runs the full v1..v10 chain on a fresh pool; nothing seeds schema_migrations at v9 with existing received_ref_updates rows before applying v10. The migration is safe (ADD COLUMN IF NOT EXISTS, nullable, Option readers), so this is coverage rather than a defect. A test that migrates to v9, inserts a row without owner_did, then applies v10 and asserts owner_did IS NULL pins the actual upgrade path.

  • [P3] Don't render peer-supplied owner_did as authoritative
    crates/gitlawb-node/src/api/events.rs:261
    Gossip and notify rows store owner_did verbatim off the wire with no signature binding it to the owner's key, and the feed echoes it. It isn't an authz break (the gate uses the matched local record, never the stored value) and it's the same spoofable class as pusher_did/node_did, but rendering the display owner from the local record for locally-known rows would close it.

The owner_did core is ready; the ipfs regression is the only merge blocker. @Gravirei recommend rebasing onto current main to drop d995c4de and pick up the #133 ipfs gate.

@Gravirei Gravirei force-pushed the fix/issue-144-full-did-wire-slug branch from b46005b to 1ddbc5b Compare July 6, 2026 04:10

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a couple of review-feedback/test issues that need to be addressed before this is ready.

Findings

  • [P2] Make the v10 migration test actually start from a v9 schema
    crates/gitlawb-node/src/db/mod.rs:3320
    This test says it simulates an existing v9 node, but it first runs db.migrate(), which applies v10 and creates received_ref_updates.owner_did before the test deletes and reseeds schema_migrations at v9. The later insert without owner_did therefore runs against a table that already has the new column, so the test would still pass if the upgrade path being claimed here was not actually exercised. Please build the pre-v10 schema without applying migration 10 first, or explicitly drop the new column/index before the reseed, then apply v10 and assert the old populated row survives with owner_did IS NULL.

  • [P3] Complete CodeRabbit's owner_did feed assertion
    crates/gitlawb-node/src/test_support.rs:2432
    CodeRabbit's request to expose owner_did in /api/v1/events/ref-updates is marked addressed, and the handler now serializes the field, but the end-to-end test still only asserts the returned repo. Since the row inserted above has owner_did: Some(owner.into()), this test would pass even if the API regressed to omitting the new field again. Please complete that review item by asserting events[0]["owner_did"] matches the inserted owner DID.

@Gravirei Gravirei requested review from beardthelion and jatmn July 6, 2026 05:26

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I found one remaining issue that needs to be addressed before this is ready.

Findings

  • [P2] Defer the unused owner_did index until the gate query lands
    crates/gitlawb-node/src/db/mod.rs:834
    Migration v10 creates idx_ref_updates_owner, but this PR only stores and projects received_ref_updates.owner_did; no current query filters, joins, or orders that table by owner_did. Since migrations run inside a transaction here, this index cannot be built concurrently, so existing nodes with populated ref-update tables take avoidable write-blocking startup work for no current read path. Please ship just the nullable column in this plumbing PR, then add the index in the follow-up that actually consumes owner_did for the feed gate.

@Gravirei Gravirei force-pushed the fix/issue-144-full-did-wire-slug branch from 1074aa4 to 4d37b11 Compare July 8, 2026 07:56

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed on dbfb0a1. Approving. I verified the security-critical properties by execution on this head.

The wire owner_did is display-only and the gate does not trust it: I seeded a private local repo and a received_ref_updates row naming it with a forged owner_did, drove the global feed as anon, and the private row stays dropped. A public repo's row with the same forged owner_did is shown with the forged value echoed. So a peer cannot use owner_did to unlock a private row, only to mislabel a public one on the feed. The shared row mapper reads owner_did as Option, so a backward-compat NULL row round-trips as None with no panic (insert_and_list_without_owner_did covers it). The v10 migration is a new highest-version block, nullable and idempotent, and its upgrade-path test genuinely starts from a v9 schema. All four emit paths carry owner_did, and I confirmed the HTTP-notify path stores it end-to-end.

Two non-blocking notes:

  • The feed echoes the wire owner_did verbatim even on rows that name a local repo this node hosts, where the true owner is already known from the record. It is display-only and joins the feed's existing untrusted peer fields (pusher_did, node_did, from_peer), so it is not a gate issue, but for local-matching rows preferring record.owner_did would avoid displaying a peer's claim over a fact the node holds. This reflects the prefer-wire resolution from an earlier round, so treat it as a judgment call rather than a required change.
  • Thin emit-path coverage: the notify_sync tests do not assert owner_did is stored, so a revert of the binding would pass all of them, and the REST None-fallback branch and the subscription projection are untested. The behavior is correct, but a regression in those paths would not be caught.

@kevincodex1 good to merge.

@beardthelion beardthelion dismissed stale reviews from themself July 8, 2026 17:25

Stale approval on an earlier head; superseded by my re-review on dbfb0a1.

@kevincodex1

Copy link
Copy Markdown
Contributor

hello @Gravirei please rebase to main and fix conflicts

@Gravirei Gravirei force-pushed the fix/issue-144-full-did-wire-slug branch from dbfb0a1 to 58d5c76 Compare July 10, 2026 03:26

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevincodex1

Copy link
Copy Markdown
Contributor

hello @Gravirei please rebase to main again and fix conflicts

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The owner_did wire plumbing is sound: it's display-only (projected after the visibility gate, which keys on the local repo record, never the peer-supplied wire value), the migration is a correct new v10 with a load-bearing upgrade test, and all four fan-out paths carry the field. Two things still gate merge.

The branch conflicts with main, and its base predates #149 (list_ref_certificates bound + upsert) and #180 (per-IP creation rate-limit), both of which touch the same files this PR does (events.rs, repos.rs, db/mod.rs, state.rs, test_support.rs). Please rebase onto current main and fix the conflicts; the resolved diff needs another pass, because a careless resolution on those files silently drops one of those merged gates.

Findings

  • [P2] Drop the per-request full-repo dedup on the ref-updates feed
    crates/gitlawb-node/src/api/events.rs:144, crates/gitlawb-node/src/graphql/query.rs:80
    list_ref_updates and the refUpdates resolver call list_all_repos_deduped() for the display fallback, but collect_visible_ref_updates already loaded that exact set, so the full-repos-table dedup CTE runs twice per request. The second load is also unconditional: it fires even when every returned row already carries a wire owner_did and the fallback is never consulted, which is the steady state once peers upgrade. The endpoint is anonymous, so a permissionless caller drives two full-table scans per hit, scaling with node repo count rather than the (<=200) rows returned. Thread the set the collector already holds through to the handler, and gate the fallback load on updates.iter().any(|u| u.owner_did.is_none()).

  • [P3] Resolve the display owner with an exact key match, not the fail-safe gate matcher
    crates/gitlawb-node/src/api/events.rs:147, crates/gitlawb-node/src/graphql/query.rs:85
    The fallback picks the first record matching ref_update_row_names_repo, a deliberately loose prefix matcher built for the visibility gate (where over-matching fails safe). For display it mis-attributes: a legacy row naming owner zX is shown with the owner of any local repo whose key prefixes zX. This is cosmetic only (the private-match case is withheld by the gate rather than disclosed) and affects legacy pre-#144 rows only. Resolve with the canonical normalize_owner_key and require a unique match, skipping on ambiguity.

@Gravirei Gravirei force-pushed the fix/issue-144-full-did-wire-slug branch from 58d5c76 to 2107633 Compare July 12, 2026 00:14
@Gravirei Gravirei requested review from beardthelion and jatmn July 12, 2026 07:37
@beardthelion beardthelion dismissed their stale review July 12, 2026 14:58

Superseded by a fresh re-review on the current head (2107633).

@beardthelion beardthelion left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the current head. The owner_did wire plumbing is sound, and I re-verified the security-critical property by execution on this head rather than reading it: the wire owner_did is display-only and never a visibility-gate input. Driving a forged wire owner_did that names a private repo through the resolver, the row stays dropped for anon and for a caller authenticated as the forged DID, and only the true owner sees it; forcing the gate open flips it to a leak, so the gate is load-bearing. The rebase is clean, #149's list_ref_certificates bound and #180's per-IP creation limiter both survive (verified by their tests). Migration is a correct new v11 (nullable, idempotent) with a load-bearing upgrade test, all four emit paths carry owner_did, and the shared mapper round-trips a null owner_did with no panic.

One item from my last pass is still open, plus two small ones.

Findings

  • [P2] Drop the redundant per-request repo dedup on the ref-updates feed. crates/gitlawb-node/src/api/events.rs:145, crates/gitlawb-node/src/graphql/query.rs:80collect_visible_ref_updates already loads list_all_repos_deduped() for the gate (events.rs:57), but both handlers load it a second time for the owner_did display fallback, so every anonymous request now runs the full-table dedup CTE twice (one before this PR). The second load is unconditional: it fires even when every returned row already carries a wire owner_did and the fallback is never consulted, which is the steady state once peers upgrade. Thread the set the collector already holds through to the handler, or gate the second load on updates.iter().any(|u| u.owner_did.is_none()).

  • [P3] Resolve the display owner with an exact key match, not the fail-safe gate matcher. crates/gitlawb-node/src/visibility.rs:229 — the legacy-None fallback reuses ref_update_row_names_repo, whose record_key.starts_with(row_key) is always true when an empty-owner-segment slug (/name) reduces row_key to "", so a legacy row can display the wrong owner. Display-only: the same over-match in the gate only ever drops (covered by feed_empty_owner_slug_matches_and_drops), and it affects only pre-#144 rows stored with owner_did = None. Resolve with normalize_owner_key and require a unique match, skipping on ambiguity.

  • [P3] Add a regression test for a forged wire owner_did. The existing gate tests forge the slug or set owner_did to None; none drives a forged wire owner_did field, which is the exact must-not (a peer-supplied owner_did must not unlock a private row). The property holds today, but a permanent test guards a future change that starts keying on the wire value. A drop-in that mirrors the existing resolver tests:

    #[sqlx::test]
    async fn forged_wire_owner_did_never_unlocks_private_row(pool: PgPool) {
        let db = db(pool).await;
        db.create_repo(&repo("r1", OWNER, "widget", false)).await.unwrap(); // private
        let mut row = ref_row("u1", "z6MkOwner/widget");
        row.owner_did = Some("did:key:z6MkAttacker".into()); // forged wire owner
        db.insert_ref_update(&row).await.unwrap();
        let schema = schema(db);
        let q = r#"{ refUpdates(repo: "z6MkOwner/widget") { refName ownerDid } }"#;
        assert_eq!(count(&anon(&schema, q).await), 0);
        assert_eq!(count(&authed(&schema, q, "did:key:z6MkAttacker").await), 0);
        assert_eq!(count(&authed(&schema, q, OWNER).await), 1);
    }

    I ran this against the branch (passes) and against a fail-open gate (fails), so it is load-bearing.

The core is solid and the security properties hold. The P2 is the one I'd want fixed before merge, and it's a small change; the two P3s are polish.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found issues that need to be addressed before this is ready.

Findings

  • [P1] Restore the iCaptcha PoW protocol support before merging
    crates/icaptcha-client/src/lib.rs:194
    This head is behind the current PR base and removes the Challenge.pow field, its solver, and powNonce from the /v1/answer request (as well as deleting pow.rs). A PoW-enabled iCaptcha service therefore has its required pow extension silently ignored and receives only {token, answer}, so it rejects the solve and gl cannot mint a proof for register/repository-create/fork writes. Rebase onto current main and preserve the optional PoW parsing, solving, nonce submission, and regression coverage.

  • [P1] Keep iCaptcha key trust scoped to the full origin
    crates/icaptcha-client/src/lib.rs:110
    x-icaptcha-url is controlled by the node, but the new host_of comparison accepts any HTTPS port on the configured operator host and marks it key_trusted. For example, an operator configured with https://icap.example:443 can be redirected to https://icap.example:8443; both challenge and answer requests then send GITLAWB_ICAPTCHA_API_KEY to that distinct listener. The base implementation compared scheme, host, and effective port specifically to prevent this. Restore full-origin matching for both acceptance and bearer-key attachment.

  • [P2] Return the received event's owner DID on the repo-scoped feed
    crates/gitlawb-node/src/api/events.rs:284
    The new field is stored on ReceivedRefUpdate, and the global REST and GraphQL feeds prefer that wire value, but this projection unconditionally substitutes record.owner_did. A received row whose slug collides with a hosted repo is consequently reported as owned by the local record rather than by its stored full DID—the exact identity ambiguity this change is intended to carry. Project u.owner_did when present and use the local value only as the legacy-None fallback, with a collision regression test.

  • [P2] Do not perform a second full repository-dedup scan for every feed read
    crates/gitlawb-node/src/api/events.rs:145
    collect_visible_ref_updates already loads list_all_repos_deduped() for its visibility gate, but the new REST projection immediately loads it again for the legacy fallback; the GraphQL resolver repeats the same pattern. This doubles the full-repository scan on every anonymous global-feed request even when all bounded result rows already contain owner_did. Thread the collector's deduped records through, or load them only if a returned legacy row actually needs fallback resolution.

Gravirei added 2 commits July 13, 2026 09:38
sha2 was at line 16 in our branch but at line 18 in upstream/main.
The 3-way merge in CI's pull/145/merge checkout kept both lines,
causing a 'duplicate key' error. Moving sha2 after tracing matches
upstream/main exactly, so the merge commit will have a single entry.
@Gravirei Gravirei requested a review from jatmn July 13, 2026 04:00

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found issues that need to be addressed before this is ready.

Findings

  • [P1] Do not persist peer-supplied owner_did as trusted event ownership
    crates/gitlawb-node/src/api/peers.rs:398
    The HTTP notify path copies req.owner_did straight into ReceivedRefUpdate, and the gossipsub receive path does the same with event.owner_did. The new REST and GraphQL projections then prefer that stored wire value whenever it is present. In deployments that still accept unsigned peer notify, any caller can set node_did to a known peer and make a visible row for zAlice/widget report owner_did: "did:key:zVictim"; even with signatures enforced, a compromised or buggy known peer can poison persisted event ownership. Because this field is presented as the full owner DID and is intended to become DID-aware feed-gate input, please either validate/bind it to the row's repo owner before storing/exposing it or treat it as untrusted and derive display ownership from trusted local state when possible.

  • [P3] Resolve legacy fallback owners with an exact unique match
    crates/gitlawb-node/src/api/events.rs:153
    The owner_did = None fallback reuses ref_update_row_names_repo, which is deliberately loose for the fail-closed visibility gate: it strips both owners to their last colon segment and accepts prefix matches. That is safe for dropping rows, but it is not safe for display attribution. For example, a legacy visible row such as /widget or alice/widget can match the first local readable widget repo whose owner key shares that empty/short/trailing segment, so REST and GraphQL can synthesize the wrong full owner DID for the exact cross-method collision class this PR is trying to disambiguate. Please use an exact/normalized, unique local match for legacy display fallback, or leave owner_did null when the owner cannot be proven.

Gravirei added 2 commits July 13, 2026 21:14
The peer-supplied owner_did (HTTP notify + gossipsub receive) and the
repo slug arrive over untrusted paths, yet REST and GraphQL projected the
wire value as trusted feed ownership.

- P1: add Db::resolve_ref_update_owner_did so a wire owner_did is echoed
  only when it matches the canonical owner of the actual local repo at that
  slug; otherwise it is dropped (returns null). The canonical DID is
  returned, never the raw wire bytes.
- P3: legacy owner_did = None rows are now attributed via an exact,
  normalized, unique local match (get_repo), not the prefix-tolerant drop
  gate, so a cross-method slug collision cannot synthesize the wrong owner.
- Unify REST (global + per-repo gossip) and GraphQL on the shared resolver
  so the two surfaces cannot drift. Add tests for forged-wire drop and
  exact-legacy attribution.
@Gravirei Gravirei requested a review from jatmn July 13, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crate:node gitlawb-node — the serving node and REST API kind:bug Defect fix — wrong or unsafe behavior subsystem:peers Peer announce, discovery, and registry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants